Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(explore): Move chart header to top of the page #19529

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Apr 5, 2022

SUMMARY

This PR implements part of chart header redesign (final design available here: #19099)

List of changes:

  • move chart header to top of the page
  • change looks and behaviour of editable title - input grows when user types text, no wrapping, when title overflows it gets truncated and tooltip with full name appears on hover
  • when title is truncated, clicking (enabling editing mode) moves the cursor to the end of the title
  • Applied additional paddings to Dataset label and column search on the left so that everything is aligned
  • Deleting the whole title is allowed - we don't revert to the last title when user clears the input. Instead, a placeholder is shown
  • When new chart is created, the default title is empty instead of "- untitled", which means that we display the placeholder
  • Editing the name of existing chart gets recorded in "Altered" table
  • if user doesn't have edit permissions, instead of an input element display span element that looks and behaves the same except for editing capabilities

Next steps:

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-04-05.at.11.35.42.mov

TESTING INSTRUCTIONS

  1. Open a chart that you have permissions to edit
  2. Click the title, verify that cursor appears
  3. When there's no text a placeholder should appear
  4. When you start typing, the input's width should adjust to its content
  5. When input is wider than the header container, it should move to the position of cursor if in edit mode, or be truncated if not in edit mode
  6. If not editing and title is truncated, display tooltip with full title on hover

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API
    CC @kasiazjc

</span>
)}
</Tooltip>
<span ref={sizerRef} className="input-sizer" aria-hidden tabIndex={-1} />
Copy link
Member Author

@kgabryje kgabryje Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That span is invisible, we need it to measure text width in order to scale input's width dynamically

@kgabryje
Copy link
Member Author

kgabryje commented Apr 5, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022

@kgabryje Ephemeral environment spinning up at http://54.201.146.200:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with a minor nit (not important): Very clean and I like this much better than the current UX. A few thoughts that came to mind when testing: it's not obvious to the user that the title has been edited. I believe some time ago it used to save it when pressing return? I'm not saying that was very intuitive, either, but since we have the pill that says when the form data has changed, maybe we should also record the changed title in it?

image

But let's leave that for a follow up, as it's not really directly related to this PR.

title={this.getSliceName()}
canEdit={
!slice ||
this.props.can_overwrite ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: while we're using the destructured slice, could we also destructure this like the others on line 224: const { ..., can_overwrite: canOverwrite } = this.props. Also maybe this.props.actions.updateChartTitle

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #19529 (0559f67) into master (90dbe8d) will increase coverage by 0.00%.
The diff coverage is 72.72%.

@@           Coverage Diff           @@
##           master   #19529   +/-   ##
=======================================
  Coverage   66.58%   66.59%           
=======================================
  Files        1677     1681    +4     
  Lines       64238    64279   +41     
  Branches     6538     6553   +15     
=======================================
+ Hits        42773    42804   +31     
- Misses      19766    19775    +9     
- Partials     1699     1700    +1     
Flag Coverage Δ
javascript 51.35% <72.72%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/src/explore/components/DatasourcePanel/index.tsx 69.23% <ø> (ø)
.../explore/components/ExploreViewContainer/index.jsx 54.39% <42.85%> (-2.60%) ⬇️
...rc/explore/components/ExploreChartHeader/index.jsx 46.03% <75.00%> (-3.97%) ⬇️
...ts/ExploreChartHeader/ChartEditableTitle/index.tsx 78.43% <78.43%> (ø)
...uperset-frontend/src/components/FaveStar/index.tsx 100.00% <100.00%> (ø)
...ntend/src/explore/components/ExploreChartPanel.jsx 74.24% <100.00%> (+2.24%) ⬆️
...re/components/controls/DatasourceControl/index.jsx 72.72% <100.00%> (+0.35%) ⬆️
...t-frontend/src/components/AsyncAceEditor/index.tsx 90.90% <0.00%> (-0.21%) ⬇️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (ø)
...ntend/src/SqlLab/components/QueryHistory/index.tsx 66.66% <0.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90dbe8d...0559f67. Read the comment docs.

@kgabryje
Copy link
Member Author

kgabryje commented Apr 5, 2022

I believe some time ago it used to save it when pressing return?

It still does 🙂 I like the idea of including the title in altered table! I'll add that to this PR

@kgabryje
Copy link
Member Author

kgabryje commented Apr 5, 2022

@villebro
image

@kgabryje kgabryje merged commit 602afba into apache:master Apr 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2022

Ephemeral environment shutdown and build artifacts deleted.

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* Move chart header to top of the page

* Implement truncating and dynamic input

* fix typing

* Prevent cmd+z undoing changes when not in edit mode

* Fix tests, add missing types

* Show changed title in altered
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants